-
Notifications
You must be signed in to change notification settings - Fork 141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix package name handling to retain version and strip ‘@’ suffix #1472
Conversation
Signed-off-by: Emin Aktas <eminaktas34@gmail.com>
Oh I need to individually "aprove and run" each check now, weird. |
@@ -226,7 +243,11 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri | |||
// package constraint including the operator. | |||
for _, pkg := range sets.List(missing) { | |||
if ver := originalPackages.versions[pkg]; ver != "" { | |||
pl = append(pl, fmt.Sprintf("%s%s", pkg, ver)) | |||
if pin := originalPackages.versions[pkg]; pin != "" { | |||
pl = append(pl, fmt.Sprintf("%s%s%s", pkg, ver, pin)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This produces an invalid world
when using tagged repositories like @local
or @edge
.
The order needs to be
- pl = append(pl, fmt.Sprintf("%s%s%s", pkg, ver, pin))
+ pl = append(pl, fmt.Sprintf("%s%s%s", pkg, pin, ver))
as per apk-world(5)
:
This is a plaintext file with one constraint using dependency notation per line. Each line has the format:
name{@tag}{[<>~=]version}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @emilylange
When I tested it with @local
package, the format you shared didn't work. This is the reason I kept that like that. However, changing the order doesn't break the local package build since it is not the case for local built packages.
With @local
package repository below example config does not work.
contents:
repositories:
- '@local ./packages'
keyring:
- melange.rsa.pub
packages:
- world@local=7.0.0-r0
This is the error message. This error happens before the changes.
2025/01/24 19:25:15 DEBU got 1 indexes:
./packages/x86_64/APKINDEX.tar.gz
Error: locking config: resolving apk packages: for arch "amd64": solving "world@local=7.0.0-r0" constraint: nothing provides "world@local=7.0.0-r0"
2025/01/24 19:25:15 INFO error during command execution: locking config: resolving apk packages: for arch "amd64": solving "world@local=7.0.0-r0" constraint: nothing provides "world@local=7.0.0-r0"
However, this config works but it doesn't match with the format.
contents:
repositories:
- '@local ./packages'
keyring:
- melange.rsa.pub
packages:
- world=7.0.0-r0@local
Can you share the failing config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was referring to the resulting /etc/apk/world
.
But you are right that this particular line I commented on isn't the culprit, as after looking a lot more into this, I can confidently say that this goes way deeper.
It is inherently confusing to have slight ordering differences across the codebase.
A minimal example and reproducer is:
contents:
repositories:
- https://dl-cdn.alpinelinux.org/alpine/edge/main
- '@testing https://dl-cdn.alpinelinux.org/alpine/edge/testing'
packages:
- alpine-base@testing
entrypoint:
command: /bin/ash -l
archs:
- amd64
Thanks to this PR here, apko lock
and apko build
are no longer failing. That's lovely and all.
But it doesn't fix the underlying cause, unfortunately.
The resulting /etc/apk/world
look something like this (v0.23.0
, based on the APKINDEX
at the time of writing):
aef8b91a311a:/# cat /etc/apk/world
alpine-base=3.22.0_alpha20250108-r0@testing
alpine-baselayout-data=3.6.8-r1
alpine-baselayout=3.6.8-r1
alpine-conf=3.19.2-r0
alpine-keys=2.5-r0
alpine-release=3.22.0_alpha20250108-r0
apk-tools=2.14.9-r0
busybox-binsh=1.37.0-r13
busybox-ifupdown=1.37.0-r13
busybox-mdev-openrc=1.37.0-r13
busybox-openrc=1.37.0-r13
busybox-suid=1.37.0-r13
busybox=1.37.0-r13
ca-certificates-bundle=20241121-r1
libcap2=2.73-r0
libcrypto3=3.3.2-r4
libssl3=3.3.2-r4
mdev-conf=4.7-r0
musl-utils=1.2.5-r9
musl=1.2.5-r9
openrc=0.56-r0
scanelf=1.3.8-r1
ssl_client=1.37.0-r13
zlib=1.3.1-r2
As pointed out in my previous comment, alpine-base=3.22.0_alpha20250108-r0@testing
is an invalid entry as per apk-world(5)
.
If you run apk upgrade
, apk fix
or apk add <some other package>
in a given container with that /etc/apk/world
, apk
will remove that entry and dependencies.
aef8b91a311a:/# apk upgrade
fetch https://dl-cdn.alpinelinux.org/alpine/edge/testing/x86_64/APKINDEX.tar.gz
fetch https://dl-cdn.alpinelinux.org/alpine/edge/main/x86_64/APKINDEX.tar.gz
(1/24) Purging alpine-base (3.22.0_alpha20250108-r0)
(2/24) Purging alpine-baselayout (3.6.8-r1)
(3/24) Purging alpine-baselayout-data (3.6.8-r1)
(4/24) Purging alpine-release (3.22.0_alpha20250108-r0)
(5/24) Purging alpine-keys (2.5-r0)
(6/24) Purging apk-tools (2.14.9-r0)
(7/24) Purging ca-certificates-bundle (20241121-r1)
(8/24) Purging busybox-mdev-openrc (1.37.0-r13)
(9/24) Purging mdev-conf (4.7-r0)
(10/24) Purging busybox-openrc (1.37.0-r13)
(11/24) Purging busybox-suid (1.37.0-r13)
(12/24) Purging musl-utils (1.2.5-r9)
(13/24) Purging scanelf (1.3.8-r1)
(14/24) Purging alpine-conf (3.19.2-r0)
(15/24) Purging openrc (0.56-r0)
(16/24) Purging busybox-binsh (1.37.0-r13)
(17/24) Purging busybox (1.37.0-r13)
(18/24) Purging libcap2 (2.73-r0)
(19/24) Purging busybox-ifupdown (1.37.0-r13)
(20/24) Purging zlib (1.3.1-r2)
(21/24) Purging ssl_client (1.37.0-r13)
(22/24) Purging libssl3 (3.3.2-r4)
(23/24) Purging libcrypto3 (3.3.2-r4)
(24/24) Purging musl (1.2.5-r9)
OK: 17592186044408 MiB in 0 packages
aef8b91a311a:/# cat /etc/apk/world
/bin/ash: cat: not found
aef8b91a311a:/# read file < /etc/apk/world ; echo $file
aef8b91a311a:/#
With alpine-base
being the only package in the example provided, apk
will delete everything, resulting in an unusable environment.
So far, I came up with the following diff to fix the underlying cause.
Though I would have to check
diff --git a/pkg/apk/apk/version.go b/pkg/apk/apk/version.go
index 185dbf7..5d88922 100644
--- a/pkg/apk/apk/version.go
+++ b/pkg/apk/apk/version.go
@@ -37,7 +37,7 @@ import (
var (
versionRegex = regexp.MustCompile(`^([0-9]+)((\.[0-9]+)*)([a-z]?)((_alpha|_beta|_pre|_rc)([0-9]*))?((_cvs|_svn|_git|_hg|_p)([0-9]*))?((-r)([0-9]+))?$`)
- packageNameRegex = regexp.MustCompile(`^([^@=><~]+)(([=><~]+)([^@]+))?(@([a-zA-Z0-9]+))?$`)
+ packageNameRegex = regexp.MustCompile(`^([^@=><~]+)(@([a-zA-Z0-9]+))?(([=><~]+)([^@]+))?$`)
)
func init() {
@@ -367,12 +367,12 @@ func resolvePackageNameVersionPin(pkgName string) parsedConstraint {
// layout: [full match, name, =version, =|>|<, version, @pin, pin]
p := parsedConstraint{
name: parts[0][1],
- version: parts[0][4],
- pin: parts[0][6],
+ pin: parts[0][3],
+ version: parts[0][6],
dep: versionAny,
}
- matcher := parts[0][3]
+ matcher := parts[0][5]
if matcher != "" {
// we have an equal
switch matcher {
diff --git a/pkg/apk/apk/version_test.go b/pkg/apk/apk/version_test.go
index 1e99665..9e6afd4 100644
--- a/pkg/apk/apk/version_test.go
+++ b/pkg/apk/apk/version_test.go
@@ -933,8 +933,8 @@ func TestResolverPackageNameVersionPin(t *testing.T) {
{"name<1.2.3", "name", "1.2.3", versionLess, ""},
{"name>=1.2.3", "name", "1.2.3", versionGreaterEqual, ""},
{"name<=1.2.3", "name", "1.2.3", versionLessEqual, ""},
- {"name@edge=1.2.3", "name@edge=1.2.3", "", versionAny, ""}, // wrong order, so just returns the whole thing
- {"name=1.2.3@community", "name", "1.2.3", versionEqual, "community"},
+ {"name@edge=1.2.3", "name", "1.2.3", versionEqual, "edge"},
+ {"name=1.2.3@community", "name=1.2.3@community", "", versionAny, ""}, // wrong order, so just returns the whole thing
}
for _, tt := range tests {
diff --git a/pkg/build/lock.go b/pkg/build/lock.go
index e00e858..c36e096 100644
--- a/pkg/build/lock.go
+++ b/pkg/build/lock.go
@@ -145,8 +145,8 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
}
// Extract pinned version if present
- if idx := strings.IndexAny(orig, "@"); idx >= 0 {
- pinned = orig[idx:]
+ if idx := strings.IndexAny(name, "@"); idx >= 0 {
+ pinned = name[idx:]
}
// Remove pinned suffix from name and version
@@ -256,10 +256,11 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
// Append all of the resolved and unified packages with an exact match
// based on the resolved version we found.
for _, pkg := range sets.List(acc.packages) {
- pkgName := fmt.Sprintf("%s=%s", pkg, acc.versions[pkg])
+ pkgName := pkg
if pin := originalPackages.pinned[pkg]; pin != "" {
pkgName = fmt.Sprintf("%s%s", pkgName, pin)
}
+ pkgName = fmt.Sprintf("%s=%s", pkgName, acc.versions[pkg])
pl = append(pl, pkgName)
}
// Sort the package list explicitly with the `=` included.
@@ -274,10 +275,11 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
for _, input := range inputs {
pl := make([]string, 0, len(input.packages))
for _, pkg := range sets.List(input.packages) {
- pkgName := fmt.Sprintf("%s=%s", pkg, input.versions[pkg])
+ pkgName := pkg
if pin := originalPackages.pinned[pkg]; pin != "" {
pkgName = fmt.Sprintf("%s%s", pkgName, pin)
}
+ pkgName = fmt.Sprintf("%s=%s", pkgName, input.versions[pkg])
pl = append(pl, pkgName)
}
// Sort the package list explicitly with the `=` included.
@@ -304,4 +306,4 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
}
// Copied from go-apk's version.go
-var packageNameRegex = regexp.MustCompile(`^([^@=><~]+)(([=><~]+)([^@]+))?(@([a-zA-Z0-9]+))?$`)
+var packageNameRegex = regexp.MustCompile(`^([^@=><~]+)(@([a-zA-Z0-9]+))?(([=><~]+)([^@]+))?$`)
Though given the lack of code coverage, this could also cause regressions in some unexpected way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emilylange Thanks for the detailed explanation! I think this needs its own ticket and maybe even a discussion in the #apko Slack channel. Since this is a merged ticket, there’s a good chance the right people might not see it. Would be great to get more visibility on this.
Fix #1471
Dear @jonjohnsonjr,
Can you please review my change? I noticed your PR here. I was also looking in the issue and this change worked.